Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[24.1] Allow drag and drop for collection elements #18699

Merged
merged 9 commits into from
Sep 16, 2024

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Aug 14, 2024

We extract the HDAObject from the DCESummary to be able to drag and drop it. For now, this works on dragging and dropping items to histories, since that process only needs the dataset's id. However, this doesn't work for DCECollections because they are dataset_collections and not HDCAs.

Drag and drop DCEs into Tool forms:

Fixes #18692

drag_drop_DCE_toolform.mp4

Drag and drop DCEs to other histories:

Note that this won't work for DatasetCollectionElements that are of type dataset_collection. DCEDatasets can be moved:

drag_drop_DCE_historypanel.mp4

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@ahmedhamidawan ahmedhamidawan marked this pull request as draft August 14, 2024 00:04
@github-actions github-actions bot added this to the 24.1 milestone Aug 14, 2024
@ahmedhamidawan ahmedhamidawan force-pushed the drag_drop_DCE branch 2 times, most recently from 793fe46 to f99d3d1 Compare August 17, 2024 00:12
@ahmedhamidawan
Copy link
Member Author

Added a new composable in commit f65df80 ; that prevents duplicate code in MultipleViewList and HistoryPanel for drag and drop operations. If this will be trouble merging forward, should we just target dev here?

@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review August 21, 2024 23:05
type = "dataset";
id = item.object.id;
} else {
throw new Error(`Invalid item type${item.element_type ? `: ${item.element_type}` : ""}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an error handler in https://github.com/galaxyproject/galaxy/pull/18699/files#diff-68e87a439364cbb0acfc54a106c0830012c8005d4288102ba12a57fc7ffcedd6L408, you might want to re-instate that. This btw can happen if the collection isn't populated yet, so we'll definitely run into this from time to time.

Copy link
Member

@mvdbeek mvdbeek Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you added a handler in https://github.com/galaxyproject/galaxy/pull/18699/files#diff-ad4aa7272cdb314a83cba0349216030549831df46d0113ef0d5b60a67d72a26aR160 ... I think it might be better to do error handling like this in the caller, so context-specific actions can be taken (e.g. Toast vs inline errors in this example). Also the caller might want to know if the action succeeded, which it will always if you catch the error.

} else {
throw new Error(`Invalid item type${item.element_type ? `: ${item.element_type}` : ""}`);
}
await copyDataset(id, historyId, type, dataSource);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you'd want to loop over the items to drag first and make sure none of them run into https://github.com/galaxyproject/galaxy/pull/18699/files#r1734725855 ? Otherwise it might be a little messy if a fraction of items is copied and another is not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the actual copy operation to another loop, so that if errors are caught for any item, we never copy anything from then on.

From what I understand, if there is any error with:

  • historyStore.createNewHistory();
  • !historyId
  • Invalid item type
  • copyDataset
  • loadHistoryById

the error would be handled and Toast-ed in useHistoryDragDrop.onDrop().

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice deduplication, thank you!

ahmedhamidawan and others added 9 commits September 3, 2024 13:01
We extract the `HDAObject` from the `DCESummary` to be able to drag and drop it.
For now, this works on dragging and dropping items to histories, since that process only needs the dataset's id. However, this doesn't work on tool forms; as we would need to get the full `HDASummary` to do that.
for now, only `getDragData` for `DCEDataset`s
This unifies the drag and drop operations in `HistoryPanel` as well as the drop area in the `MultipleViewList`.
@mvdbeek mvdbeek merged commit aad1caf into galaxyproject:release_24.1 Sep 16, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants